Skip to content

security: apply the Windows ACL protection to the workspace trust store - #250

Open
Dhevenddra wants to merge 1 commit into
andrewyng:mainfrom
Dhevenddra:fix/workspace-trust-windows-acl
Open

security: apply the Windows ACL protection to the workspace trust store#250
Dhevenddra wants to merge 1 commit into
andrewyng:mainfrom
Dhevenddra:fix/workspace-trust-windows-acl

Conversation

@Dhevenddra

Copy link
Copy Markdown

Problem

WorkspaceTrustStore.set_trusted() protected workspace_trust.json with a bare
os.chmod(tmp, 0o600). On Windows that is a silent no-op, because os.chmod there only
toggles the read-only attribute. The file therefore keeps whatever ACEs it inherits from its
parent directory.

coworker/secrets.py:62-66 already documents this exact hazard:

POSIX expresses this with mode bits (0700 dir / 0600 file). Windows has no such bits —
os.chmod there only toggles the read-only flag, so a 0600 chmod is a silent no-op and
the file inherits broad ACLs (SYSTEM, Administrators, …). Use an ACL instead: strip
inherited entries and grant the current user alone.

It solves the problem in _restrict_to_user(), exposed as the write_private_text() helper.
server/run.py:134 uses that helper for the sidecar auth token. The trust store did not.

This matters because workspace_trust.json is the allowlist that decides which workspaces get
auto-approved command execution. test_workspace_command_trust_controls_live_engine asserts
that trusting a workspace flips run_shell from needs_user to allowed.

Evidence

Measured on Windows 11, comparing the two "user-only" files the app writes:

File Mechanism st_mode Actual DACL
sidecar-9999.token write_private_text() 0o666 <user>:(F)
workspace_trust.json (before) os.chmod(0o600) 0o666 NT AUTHORITY\SYSTEM:(I)(F), BUILTIN\Administrators:(I)(F), <user>:(I)(F)
workspace_trust.json (after) write_private_text() 0o666 <user>:(F)

The (I) flags confirm those ACEs were inherited, so the file was never restricted. That is
exactly what the comment in secrets.py predicts.

On the scope of this, stated plainly: on a single-user machine the extra principals are SYSTEM
and Administrators, which are already privileged, so this is not a straightforward privilege
escalation. It matters on shared, domain-joined or managed machines, where
BUILTIN\Administrators can include accounts the user does not personally control. It also
matters as a plain correctness defect, because the code claims a protection it does not deliver
on one of the two shipped desktop platforms.

Why the existing tests did not catch it

Both "user-only" tests assert a POSIX bitmask:

assert (path.stat().st_mode & 0o777) == 0o600

On Windows os.stat derives st_mode from the read-only attribute alone and has no view of
ACLs, so every writable file reports 0o666. That assertion is unsatisfiable on Windows even
when the file is correctly protected: it fails for the properly restricted sidecar token too. It
cannot tell a locked-down file apart from a world-readable one.

I replaced it with a shared assert_user_only() in tests/conftest.py, which checks mode bits
on POSIX and the DACL on Windows.

Verification

Windows 11, Python 3.11.9:

Stage test_config (trust store) test_server (token)
Before any change fail fail
Test fix only still fails, on the real defect pass
Test fix plus source fix pass pass

The middle row is the useful one. With only the assertion corrected, the new check passes the
genuinely protected token file and still fails the trust store, which is what shows it
discriminates rather than just passing everything.

Full suite on Windows went from 9 failed, 880 passed to 7 failed, 882 passed. The remaining
7 are pre-existing Windows failures unrelated to this change: four in test_slack_relay plus one
in test_github_installs (all timeouts), test_workspace_command_trust_controls_live_engine
hitting WinError 32, and test_ui_refresh_e2e. I am happy to file those separately. The relay
timeouts in particular have a clean root cause I can write up.

Linux, Python 3.12, matching CI, run in Docker: both target tests pass.

Notes for review

Interaction with #186: that PR closes the TOCTOU inside write_private_text(). There is no file
overlap with this PR, since it touches secrets.py and this one touches workspace_trust.py.
Routing the trust store through the shared helper means it picks up that fix automatically once
#186 lands, rather than needing a second patch.

One deliberate behaviour change: the temp file name goes from .{name}.{pid}.tmp to
{name}.tmp, because that is what write_private_text() uses. This drops pid-scoping for
concurrent writers. I judged consistency with the existing helper, and with the more sensitive
token file that already accepts this, to be worth more than pid-scoping for a file written by a
single desktop process. I am happy to keep the pid-scoped name and call _restrict_to_user()
directly instead if you would prefer.

write_private_text() also chmods the parent directory to 0700 on POSIX. state_dir() is
already 0700 via the SecretStore, so that is a no-op in practice.

Parsing icacls output inside a test is not elegant. The alternative is a pywin32 dependency
for win32security. Since secrets.py:80 already shells out to icacls, this at least stays
consistent with existing practice, and it is easy to swap if you would rather take the
dependency.

Context

I found this while running the test suite on Windows. CI is ubuntu-latest only, so this class
of defect cannot currently be caught upstream. I would be glad to follow up with a Windows leg on
the CI matrix once the handful of pre-existing Windows failures are cleared.

WorkspaceTrustStore.set_trusted protected its state file with a bare
os.chmod(tmp, 0o600). On Windows that is a silent no-op, because os.chmod
only toggles the read-only bit there, so workspace_trust.json kept the broad
ACEs it inherited from its parent directory:

    NT AUTHORITY\SYSTEM:(I)(F)
    BUILTIN\Administrators:(I)(F)
    <user>:(I)(F)

coworker/secrets.py already documents this hazard and solves it in
_restrict_to_user() via icacls /inheritance:r /grant:r, exposed as the
write_private_text() helper. server/run.py uses that helper for the sidecar
auth token, but the trust store did not. Since this file is the allowlist
deciding which workspaces get auto-approved command execution, route it
through the same helper. Afterwards the file carries a single ACE:

    <user>:(F)

The two tests meant to catch this asserted st_mode & 0o777 == 0o600, which is
unsatisfiable on Windows. os.stat cannot see ACLs there, so every writable
file reports 0o666, including the correctly protected sidecar token. Replace
both with a shared assert_user_only() that checks mode bits on POSIX and the
DACL on Windows.

@rajpratham1 rajpratham1 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pull request improves the security of the workspace trust store by replacing the custom temporary file and chmod logic with the shared write_private_text helper. This ensures the trust store receives the same atomic write behavior and platform-specific file protection already used for sensitive secrets, including proper ACL restrictions on Windows where os.chmod(0o600) alone is insufficient. The test suite is updated with a platform-aware helper that validates file protection using POSIX mode bits on Unix-like systems and ACL inspection on Windows, with existing tests updated to use the shared assertion. Based on the visible changes, the implementation is consistent with the existing security model and no blocking issues are apparent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants